fix(babel-preset): preserve Platform.select initializers (#58350) - #58442

@vzaidman has exported this pull request. If you are a Meta employee, you can view the originating Diff in D119484692.

@vzaidman Hello, I'd like to push back on this one a bit.

Firstly, on the idea itself. Platform.select picks a value. IMO the arms are object values because that's how you use the api to select a value, not because they were meant to run side-effectful code.

I ran the branch through Metro to see what it actually does:

It doesn't change anything for normal app code. Metro sets inlinePlatform: true and always adds its own inline plugin. That plugin matches Platform.select as well, and it has no purity check. So when the new check bails out, the call just gets collapsed by Metro a bit later anyway. Here's Platform.select({ios: iosSide(), android: androidSide()}) built for Android with this branch applied:

dev   var v = androidSide();
prod  var v = androidSide();

Dev and prod already behave the same. Metro's plugin runs in dev too, so Platform.select is collapsed there as well. It's not something that only breaks in prod.

This affects even RN itself. Two files import Platform with a relative path, which Metro's matcher doesn't recognise, so nothing collapses them once the new check bails: SafeAreaView and nativeImageSource. For SafeAreaView that means Metro now picks up ./RCTSafeAreaViewNativeComponent as a dependency, so Android bundles and runs an iOS-only spec.

I understand the motivation, but the point of this plugin is to collapse platform branches. This check makes it quietly stop doing that for a whole class of ordinary code: any arm that is a property read, like Platform.select({ios: fonts.bold, default: fonts.medium}). scope.isPure treats every member expression as impure, since a getter could in theory do something, even though none of these typically do. The point is that the day Metro's matcher goes (its removal is the stated plan in 40c0612), the inliner starts silently giving up on code with no side effects in it at all, and nothing tells you it happened.

Both examples above show the change can grow the bundle. The RN ones are also a correctness problem: the app ends up shipping and running code meant for another platform.

So my ask is to not land this at all. Dropping the other platform's arm is the point of the inliner, not a bug. The only thing this changes today is that two RN files start shipping iOS code to Android.

Btw, Expo has been doing the unconditional version of this. It turns Metro's plugin off and uses its own, which runs in prod only and collapses Platform.select with no purity check. I'm not aware of that causing issues since it was merged in March 2024.

Thanks for your consideration!

While we really need to consider all these points for the future of the plugin, in this point, we don't want to introduce breaking changes, so it has to be added back in. WDYT @robhogan ?

Yeah, having looked a bit closer at what scope.isPure does I agree with @vonovak - it's too conservative to the point of rendering Platform.select ineffective in cases that will surprise people. Even Flow doesn't treat prop access as impure.

I don't think the plugin should collapse all branches regardless though. This isn't a plugin users opt into or even know about, necessarily, it's a release-mode optimisation on top of an API (Platform.select()) that's useful on its own - so we should be careful - but isPure goes too far.

For a future version, maybe we should have some warning/feedback when we bail out of optimisations, and then we can ramp up the strictness? One for the DevX WG tomorrow..

we don't want to introduce breaking changes, so it has to be added back in

Could you elaborate on that? Is that because this plugin catches more candidates than the Metro one does/did?

@robhogan

we don't want to introduce breaking changes, so it has to be added back in

Platform.select({
  ios: selected(),
  android: discarded(),
});

Was previously executing both effects, but with the inliner, suddenly only one will be executed. This is a breaking change.

Same goes for a.b.c where getters could in theory execute side effects.

I agree that preferably we should collapse all these branches, but maybe with some sort of warning or other way of letting users know their code behaviour changes.

Am I right that the inliner is enabled by default? If not then this is more trivial to do.

Was previously executing both effects, but with the inliner, suddenly only one will be executed. This is a breaking change.

There’s no previous version where we executed both selectors in release builds. Before RN 0.88, Metro inlined that pattern and elided discarded(). The only difference now is we’ve moved the plugin into the RN repo, but it’s still only activated by Metro via the Babel caller API.

Yeah, you could see this as a bug, but it’s a very longstanding behaviour, not a regression that needs an urgent fix, IIUC - not at the expense of the usefulness of the API.